Skip to content

Enabling passing context into Model.forecast v2 #888

Merged
merged 17 commits into from
Sep 2, 2022
Merged

Conversation

Mr-Geekman
Copy link
Contributor

@Mr-Geekman Mr-Geekman commented Aug 25, 2022

Before submitting (must do checklist)

  • Did you read the contribution guide?
  • Did you update the docs? We use Numpy format for all the methods and classes.
  • Did you write any new necessary tests?
  • Did you update the CHANGELOG?

Proposed Changes

  1. Change signatures of base models.
  2. Add interfaces for models with intervals and models with context.
  3. Fix base models to work with different interfaces differently.

Closing issues

Closes #845.

@Mr-Geekman Mr-Geekman self-assigned this Aug 25, 2022
@github-actions
Copy link

github-actions bot commented Aug 25, 2022

🚀 Deployed on https://deploy-preview-888--etna-docs.netlify.app

@github-actions github-actions bot temporarily deployed to pull request August 25, 2022 16:03 Inactive
@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2022

Codecov Report

❗ No coverage uploaded for pull request base (inference@17db427). Click here to learn what that means.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             inference     #888   +/-   ##
============================================
  Coverage             ?   49.58%           
============================================
  Files                ?      130           
  Lines                ?     7501           
  Branches             ?        0           
============================================
  Hits                 ?     3719           
  Misses               ?     3782           
  Partials             ?        0           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions github-actions bot temporarily deployed to pull request August 25, 2022 16:38 Inactive
etna/models/base.py Show resolved Hide resolved
etna/models/base.py Show resolved Hide resolved
etna/models/base.py Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to pull request August 29, 2022 10:02 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 31, 2022 10:27 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 31, 2022 10:35 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 31, 2022 16:05 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 31, 2022 16:18 Inactive
Copy link
Collaborator

@alex-hse-repository alex-hse-repository left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need tests for the correct dispatching in pipelines

etna/models/base.py Show resolved Hide resolved
etna/models/base.py Outdated Show resolved Hide resolved
etna/models/base.py Outdated Show resolved Hide resolved
etna/models/base.py Show resolved Hide resolved
etna/models/base.py Show resolved Hide resolved
etna/models/nn/tft.py Outdated Show resolved Hide resolved
etna/models/seasonal_ma.py Outdated Show resolved Hide resolved
etna/models/base.py Outdated Show resolved Hide resolved
etna/pipeline/autoregressive_pipeline.py Outdated Show resolved Hide resolved
etna/pipeline/pipeline.py Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to pull request September 1, 2022 08:52 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 1, 2022 13:32 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 1, 2022 13:36 Inactive
etna/pipeline/pipeline.py Outdated Show resolved Hide resolved
etna/pipeline/pipeline.py Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to pull request September 2, 2022 09:40 Inactive
@Mr-Geekman Mr-Geekman marked this pull request as ready for review September 2, 2022 09:56
@github-actions github-actions bot temporarily deployed to pull request September 2, 2022 10:04 Inactive
self.model = cast(ContextRequiredModelType, self.model)
if isinstance(self.model, DeepBaseModel):
current_ts_forecast = current_ts.make_future(
future_steps=self.model.decoder_length, tail_steps=self.model.encoder_length
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.model.context_size $\neq$ self.model.encoder_length ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For DeepBaseModel they are the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we use extra branch in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of self.model.decoder_length.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, we should fix it in the future


if isinstance(self.model, get_args(ContextRequiredModelType)):
self.model = cast(ContextRequiredModelType, self.model)
if isinstance(self.model, DeepBaseModel):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nested ifs are not readable. Better to use plain if cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@github-actions github-actions bot temporarily deployed to pull request September 2, 2022 11:49 Inactive
@Mr-Geekman Mr-Geekman merged commit 2a207d2 into inference Sep 2, 2022
@Mr-Geekman Mr-Geekman deleted the issue-845-v2 branch September 2, 2022 13:59
@Mr-Geekman Mr-Geekman mentioned this pull request Oct 26, 2022
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants